Refactor ethscription transaction building architecture#104
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the ethscription transaction building architecture by consolidating logic into a unified builder pattern and introducing specialized transaction classes. The refactoring improves separation of concerns by moving ethscription detection and transaction building logic into focused, single-responsibility classes.
Key changes:
- Replaces EthscriptionDetector with integrated detection in EthscriptionTransactionBuilder
- Creates specialized L1AttributesTransaction class for system attributes transactions
- Refactors EthscriptionTransaction to use factory methods and dynamic property generation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/services/geth_driver.rb | Updates reference to use SysConfig::SYSTEM_ADDRESS instead of EthscriptionTransaction::SYSTEM_ADDRESS |
| app/models/l1_attributes_transaction.rb | New specialized class for handling L1 attributes transactions with deposit payload generation |
| app/models/ethscriptions_block.rb | Updates to use new L1AttributesTransaction factory method |
| app/models/ethscription_transaction_builder.rb | Comprehensive refactor integrating detection logic and using factory methods for transaction creation |
| app/models/ethscription_transaction.rb | Major refactor to use factory methods, dynamic properties, and simplified structure |
| app/models/ethscription_detector.rb | File deleted as logic moved to EthscriptionTransactionBuilder |
| app/models/eth_transaction.rb | Updates method call to use EthscriptionTransactionBuilder directly |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| class EthscriptionTransactionBuilder | ||
| include SysConfig | ||
|
|
||
| # Event signatures | ||
| ESIP1_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscription(address,bytes32)').unpack1('H*') | ||
| ESIP2_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscriptionForPreviousOwner(address,address,bytes32)').unpack1('H*') | ||
| CREATE_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_CreateEthscription(address,string)').unpack1('H*') |
There was a problem hiding this comment.
Computing Keccak256 hashes at class load time is inefficient. Consider memoizing these constants using a lazy initialization pattern or moving them to a module constant.
| class EthscriptionTransactionBuilder | |
| include SysConfig | |
| # Event signatures | |
| ESIP1_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscription(address,bytes32)').unpack1('H*') | |
| ESIP2_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscriptionForPreviousOwner(address,address,bytes32)').unpack1('H*') | |
| CREATE_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_CreateEthscription(address,string)').unpack1('H*') | |
| # Memoized event signature constants to avoid recomputation at class load time | |
| ESIP1_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscription(address,bytes32)').unpack1('H*') | |
| ESIP2_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_TransferEthscriptionForPreviousOwner(address,address,bytes32)').unpack1('H*') | |
| CREATE_SIG = '0x' + Eth::Util.keccak256('ethscriptions_protocol_CreateEthscription(address,string)').unpack1('H*') | |
| class EthscriptionTransactionBuilder | |
| include SysConfig | |
| # Event signatures | |
| ESIP1_SIG = ::ESIP1_SIG | |
| ESIP2_SIG = ::ESIP2_SIG | |
| CREATE_SIG = ::CREATE_SIG |
| def seen_creates | ||
| @seen_creates_mutex ||= Mutex.new | ||
| return @seen_creates if @seen_creates | ||
|
|
||
| @seen_creates_mutex.synchronize do | ||
| @seen_creates ||= Concurrent::Set.new | ||
| end |
There was a problem hiding this comment.
The mutex assignment @seen_creates_mutex ||= Mutex.new is not thread-safe. Multiple threads could create different mutex instances. Initialize the mutex in a class-level synchronized block or use a class variable.
| def input | ||
| return @cached_input if defined?(@cached_input) | ||
|
|
||
| @cached_input = case ethscription_operation |
There was a problem hiding this comment.
The caching mechanism using @cached_input is not thread-safe. Multiple threads could compute and overwrite the cached value simultaneously. Consider using proper synchronization or making this computation idempotent.
No description provided.